Fix JAR resource loading#13
Conversation
- Load defined populator file - Switch to manual ZIP resource handling in search function
There was a problem hiding this comment.
Thanks @art-and-co, for your contribution!
It is definitely valuable!
See my comments for further improvements, maybe.
I really would like to know your opinion about my slight changes to your proposal.
If you agree, just take them over in your PR.
|
@art-and-co , one further note: |
|
I will try to add 3 test cases for
Loading default It would be nice to cover JAR reading logic as well but this requires a bit of efforts: JAR generation and etc. So I would prefer to leave it out of scope. Currently I got tests running locally. Now I need to understand tests structure better. |
- Added cases to cover load method
|
Added mentioned test cases. Please, review. Now I copy-pasted some test logic. Maybe it could be extracted into separate method? |
This reverts commit 5527ebc.
kuniss
left a comment
There was a problem hiding this comment.
Thanks for taking the effort to add some tests.
I have one question, however. See my other comment.
src/test/java/jpos/config/simple/xml/JavaxRegPopulatorTestCase.java
Outdated
Show resolved
Hide resolved
kuniss
left a comment
There was a problem hiding this comment.
I will merge the changes shortly.
|
A first release candidate version of 4.0.2 with this PR contained is available at Sonatype's snapshot repository https://oss.sonatype.org/content/repositories/snapshots/. <dependency>
<groupId>org.javapos</groupId>
<artifactId>javapos-config-loader</artifactId>
<version>4.0.2-SNAPSHOT</version>
</dependency>@art-and-co, please test. |
|
Thank you, @kuniss, it was a pleasure to have a code review with you. |
|
You are welcome! Me too. |
|
@art-and-co , what do you think, when you will be able to test the release candidate from above? |
|
Yes, I will test it and will report if I would find any issues. Thank you! As well I have found that |
I'm not sure whether I have understood you correctly, but when the version of the The |
|
Previously when I have looked at Maven |
As I said, |
|
@art-and-co, it seems, the one issue you found and corrected was already tested successfully: See here: JavaPOSWorkingGroup/javapos#9 (comment). |
|
I am happy to hear that it was tested by somebody else too. It is not clear where I have tested locally the changes before creation of this PR. There were some changes introduced during PR, which were not fully tested yet by me. I plan to do some additional tests this week, which would cover the mentioned changes as well. As well I will use I will report results here. I am looking forward to see JCL v4.0.2 used in |
|
@kuniss, finally, I have tested with the new JCL version using available online snapshot version. I have build I have verified that generated source JAR contains correct modifications. I have run this version on Windows 10 using Java 11. I was running in normal and debug mode. I have tested both cases:
In ZIP stream logic it was properly loading The testing has been successful. |
|
Great! I will build and publish it tomorrow then. |
|
NB Today I have tested on Windows 10 Java 1.8. All run OK. |
|
javapos-config-loader-4.0.2 release is available on Maven Central: https://central.sonatype.com/artifact/org.javapos/javapos-config-loader/4.0.2 According javapos release incorporating too: https://central.sonatype.com/artifact/org.javapos/javapos/1.15.6 |
Currently there are following issues with JCL v4+:
Reversed Populator Logic
This line is incorrect:
Instead of loading the defined populator file it loads default and vice versa.
JCL Cannot Load Found JAR Resource Files
JCL v4 uses
try-with-resourcesinfindFileInJarZipFilesmethod:As result this statement is also closing returned
zipEntryfile handle too.To correct this
tryandfinallywere used instead.NB: It seems my Eclipse settings have screwed whitespace with tabs a little bit.